Conversation
Codecov Report
@@ Coverage Diff @@
## master #64 +/- ##
==========================================
- Coverage 96.9% 96.89% -0.01%
==========================================
Files 15 15
Lines 905 903 -2
Branches 183 183
==========================================
- Hits 877 875 -2
Misses 28 28
Continue to review full report at Codecov.
|
|
FYI @sergeyzenchenko |
sergeyzenchenko
left a comment
sergeyzenchenko
left a comment
There was a problem hiding this comment.
LGTM, but left one comment about sorting. Take a look at it
| encodeMap(object: Record<string, unknown>, depth: number) { | ||
| const size = this.countObjectKeys(object); | ||
| const keys = Object.keys(object); | ||
| if (this.sortKeys) { |
There was a problem hiding this comment.
Maybe we should have two separate branches? One with keys array and one without? it will be better from performance perspective. But maybe it's over optimization
There was a problem hiding this comment.
Maybe you are right because most of the cases do not set sortKeys.
There was a problem hiding this comment.
Unfortunately, Object.keys() version is always faster.
benchmark script:
import { encode, decode } from "../src";
// @ts-ignore
import _ from "lodash";
const data: Record<string, number> = {};
for (let i = 0; i < 100; i++) {
data["foo" + i] = i;
}
// warm up
const encoded = encode(data);
decode(encoded);
// run
console.time("encode sortKeys: false");
for (let i = 0; i < 100000; i++) {
encode(data, { sortKeys: false });
}
console.timeEnd("encode sortKeys: false");
console.time("encode sortKeys: true");
for (let i = 0; i < 100000; i++) {
encode(data, { sortKeys: true });
}
console.timeEnd("encode sortKeys: true");result of for-in version:
$ npx ts-node benchmark/sandbox.ts
encode sortKeys: false: 2681.488ms
encode sortKeys: true: 2374.899ms
result of Object.keys() version (i.e. the same as this PR):
$ npx ts-node benchmark/sandbox.ts
encode sortKeys: false: 1336.151ms
encode sortKeys: true: 2343.010ms
I think for-in version uses less memory, but it's much slower.
So I don't change this PR.
3 participants
sortKeys